Improve TLS certificate loading, handling and validation#478
Merged
Conversation
1. Improved Certificate Loading: - Now fails explicitly when no certificates can be loaded - Properly reports when CA certificates are unavailable 2. Enhanced Error Handling: - Added new error types for specific TLS/certificate validation issues - Improved error messages for better diagnostics 3. Better Certificate Validation: - Implemented certificate validation checks during connection establishment - Added basic validation of certificate data to ensure certificates aren't empty - Added proper error handling for invalid DNS names 4. Proper TLS Connection Validation: - Added explicit error handling for TLS connection failures - Added logging for certificate validation failures for easier debugging
cceckman-at-fastly
previously approved these changes
May 28, 2025
Contributor
cceckman-at-fastly
left a comment
cceckman-at-fastly
left a comment
There was a problem hiding this comment.
Patched with some syntactic changes - map_err and use of the ?, mostly - to cut out some of the error-handling boilerplate.
@jedisct1 If this looks good to you, it's good to merge IMO!
kailan
reviewed
Apr 20, 2026
Comment on lines
49
to
70
| let mut roots = rustls::RootCertStore::empty(); | ||
| match rustls_native_certs::load_native_certs() { | ||
| Ok(certs) => { | ||
| let mut cert_added = false; | ||
| for cert in certs { | ||
| if let Err(e) = roots.add(&rustls::Certificate(cert.0)) { | ||
| warn!("failed to load certificate: {e}"); | ||
| match roots.add(&rustls::Certificate(cert.0)) { | ||
| Ok(_) => cert_added = true, | ||
| Err(e) => { | ||
| // Log but continue trying other certs | ||
| warn!("failed to load certificate: {e}"); | ||
| } | ||
| } | ||
| } | ||
| if !cert_added { | ||
| return Err(Error::TlsNoCertsAdded); | ||
| } | ||
| } | ||
| Err(err) => return Err(Error::BadCerts(err)), | ||
| } | ||
| if roots.is_empty() { | ||
| warn!("no CA certificates available"); | ||
| return Err(Error::TlsNoCAAvailable); | ||
| } |
Member
There was a problem hiding this comment.
I know some of this was pre-existing but I believe we can simplify to:
let certs = rustls_native_certs::load_native_certs().map_err(Error::BadCerts)?;
let mut roots = rustls::RootCertStore::empty();
let (added, failed) = roots.add_parsable_certificates(&certs.into_iter().map(|c| c.0).collect::<Vec<_>>());
if failed > 0 {
warn!("failed to load {} certificate(s). attempting to continue with {} available certificate(s)", failed, added);
}
if roots.is_empty() {
return Err(Error::TlsNoCAAvailable);
}
Member
There was a problem hiding this comment.
This also means the TlsNoCertsAdded error variant can be removed
kailan
reviewed
Apr 20, 2026
Comment on lines
+170
to
+172
| let tcp = connect_fut | ||
| .await | ||
| .map_err(|e| std::io::Error::other(format!("TCP connection error: {}", e)))?; |
Member
There was a problem hiding this comment.
The HTTP connector already provides descriptive error messages so we probably don't need this extra wrapping
Suggested change
| let tcp = connect_fut | |
| .await | |
| .map_err(|e| std::io::Error::other(format!("TCP connection error: {}", e)))?; | |
| let tcp = connect_fut.await? |
Member
|
@jedisct1 I hope you don't mind that I took the liberty of reviving this pull request and taking advantage of some since-released Rust features that can simplify the error construction. I'll re-request review from both parties and hopefully we can ship your changes asap. |
kailan
approved these changes
Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improved Certificate Loading:
Better Certificate Validation:
Proper TLS Connection Validation: